(Closes #3367) statement next_accesses - #3560

I've not implemented the tests for this yet - I'm not sure if there should be any specific order to the result (or whether that should matter). I'm wondering if it should be sorted by abs_position or not.

I do think that for some blocks the way this has to be implemented at current will be prohibitively expensive, but the only solution to that will be a rewrite of the DUCs.

However, for the LFRic halo exchange infrastructure that requires this code I'm hopeful its not too expensive.

Looking at #3399, the way I implemented this for Assignments was to return a dictionary of Signature, List[Node] (so multiple lists for each variable in the Assignment).

In this PR I thought that distinguishing between them is not so relevant, the caller should be worrying about the result, and next/previous_accesses should always return just list[Node] to be consistent regardless of node type (where supported).

@arporter @sergisiso Do you agree? If so I'll modify Assignment before I continue with this implementation (as its currently stuck in testing due to this).

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (c937912) to head (3b649e3).

@@            Coverage Diff             @@
##            master     #3560    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          403       403            
  Lines        57394     57604   +210     
==========================================
+ Hits         57394     57604   +210     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

It would be good to have a reminder/plan about how we are going to use this to do the HEx with the DUC.

Does completing #3124 changes the need for these? For instance in the loop implementation you have self.variable_reference.next_accesses() ... but this should not be necessary anymore now that this is a children?

Are this methods equivalent to:
accesses = {ref.next_accesses() for ref in self.walk(Reference)}
?

It would be good to have a reminder/plan about how we are going to use this to do the HEx with the DUC.

Does completing #3124 changes the need for these? For instance in the loop implementation you have self.variable_reference.next_accesses() ... but this should not be necessary anymore now that this is a children?

I don't remember to be honest, I think we need Kern to be a Call and then it inherits next_access and should "just work" to reimplement forward_dependence and backward_depedence (that I understood are the functions used by the HEx?) to use this functionality.

I could just do for child in loop.children probably, I currently didn't consider the start/stop/step which is probably wrong, so I should fix that up.

Are this methods equivalent to: accesses = {ref.next_accesses() for ref in self.walk(Reference)} ?

Not quite, it should be similar, but with node equality rules that wouldn't quite work I think. It also doesn't return any accesses contained inside the Statement itself, which for Loops would occur for accessing those references individually.

Swapping Assignment to just return a singular list was easy and didn't cause any side effects anywhere that I can see (as it was done in preperation for this PR I believe) so I've made that change and will continue to implement tests, as I think they'll make it clearer.

Not quite, it should be similar, but with node equality rules that wouldn't quite work I think. It also doesn't return any accesses contained inside the Statement itself, which for Loops would occur for accessing those references individually.

I'm a bit lost by this, could you show an example of an statement where it would have different results?

Not quite, it should be similar, but with node equality rules that wouldn't quite work I think. It also doesn't return any accesses contained inside the Statement itself, which for Loops would occur for accessing those references individually.

I'm a bit lost by this, could you show an example of an statement where it would have different results?

do i = 1, 100
   j = 4
end do
j = 3

If you just do psyir.walk(Assignment)[0].next_accesses() you'll get a self-reference to j=4 and then j=3. The loop.next_accesses() will just give the reference in j=3.

I don't remember to be honest, I think we need Kern to be a Call and then it inherits next_access and should "just work" to reimplement forward_dependence and backward_depedence (that I understood are the functions used by the HEx?) to use this functionality.

My worry is that next_accesses would return the ReadAfterRead without a way to differenciate them? In the Reference.next_accesses it is not a problem because we can do the is_read/is_write of both the location and target. But in a next_accesses of the statement we cannot do that.

If you just do psyir.walk(Assignment)[0].next_accesses() you'll get a self-reference to j=4 and then j=3. The loop.next_accesses() will just give the reference in j=3.

Then maybe something like:

accesses = {ref.next_accesses() for ref in self.walk(Reference)}
accesses = filter(lambda x: not x.is_descendant_of(self), accesses)

I don't remember to be honest, I think we need Kern to be a Call and then it inherits next_access and should "just work" to reimplement forward_dependence and backward_depedence (that I understood are the functions used by the HEx?) to use this functionality.

My worry is that next_accesses would return the ReadAfterRead without a way to differenciate them? In the Reference.next_accesses it is not a problem because we can do the is_read/is_write of both the location and target. But in a next_accesses of the statement we cannot do that.

If you just do psyir.walk(Assignment)[0].next_accesses() you'll get a self-reference to j=4 and then j=3. The loop.next_accesses() will just give the reference in j=3.

I guess I don't remember if we have some way to check if a Statement has read/write access to a variable with reference_accesses anyway? I think thats sufficient to distinguish between WaR and RaR.

Then maybe something like:

accesses = {ref.next_accesses() for ref in self.walk(Reference)}
accesses = filter(lambda x: not x.is_descendant_of(self), accesses)

Because nodes aren't hashable this doesn't work, we can't put them in sets.

Also calling ref.next_accesses() repeatedly is currently inefficient (until the DUC rewrite) as assignment.next_accesses() is more efficient where possible, and I can probably do a similar trick for some of loop's nodes later.

Been a bit slow with this sorry, a first implementation is ready for a first look @sergisiso @arporter . I do (at some point during the review process) want to look at potentially improving the performance of the loop implementation, and maybe look at some of the DSL nodes implementations (may need some advice there), but a first review to see how it works would be better first I think.

@sergisiso Not everything is fixed (far from it) but can you look at the new implementations of Statement and Loop and let me know if you're ahppy with these changes? If so, I'll continue with those changes.

@sergisiso I think this is ready for a first proper look now.

Having two different behaviours of these methods, some find their children's references (with their own logic for performance reasons) and some don't (they simply return [] even when they have children). If it were up to me I would prefer a single behaviour. But without use cases its difficult to say what will be useful in practice.

Some of the domain-specific nodes I wasn't sure whether it made sense to look at their children or not, so my initial implementation was left as [], whilst some need to specifically avoid their children (e.g. StandaloneDirective). I suspect the Kern implementation can either be improved or is wrong, but I felt that might be a decision better made later when we have a more concrete plan for how they should be integrated fully with PSyIR functionality (e..g Kern subclassing Call). I'm also never quite sure if the Kern can use the domain-specific knowledge or not?

I went for an abstractmethod as a defensive programming measure mostly - it prevents the creation of Statement subclasses without this method being implemented and forces a specific decision to be made about their behaviour, even if that behaviour is just to use the parent class' implementation.

• I don't particularly like that these are abstract methods but many of their implementations are just return super().previous_accesses(). I think I would make them not abstract, and delete all the empty implementations. But again this makes more sense with a single behaviour. I think you decided to use an abstract method to force each node to choose which behaviour they have? But this adds lots of verbosity.

I can see both arguments (defensive programming versus verbosity) but am inclined to agree with Sergi here - i.e. remove the abstract property and remove the empty implementations. We don't add new Node types very often after all.

Regarding the "returns different things", I'm more relaxed. However, the DSL-level nodes should probably raise a NotImplementedError rather than silently do the wrong thing. I appreciate that this might get triggered unexpectedly though so it's a suck-it-and-see.

@sergisiso @arporter I think this is ready for another look now.